-
Notifications
You must be signed in to change notification settings - Fork 504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate old Callback::Call #733
Conversation
Regarding Until now, |
efd4464
to
c82a3af
Compare
this sounds useful for the need to perform synchronous/blocking operations
interesting question, but I can tell you it would be unexpected if the |
@kkoopa @reqshark Those are good points! Thanks for bringing those up.
Yeah, my PR downplays the use of The motivation here was that calling asynchronously without providing the async context is problematic, so let's deprecate things to ensure the native module ecosystem can migrate to the 'correct' mechanism. At the same time, it is valid to have native modules that might be using Thinking about this some more, Nan provides two mechanisms to do a 'call' the function:
Would it make sense to add a
I think that is a good question, but perhaps we should discuss this in a separate thread? It is a use-case that is currently unsupported. |
Yes, that could work. I believe it was suggested in the original ticket that wanted callbacks without ticks. I would also not be fully opposed to making |
I now remembered that |
Yes, that is precisely what I had in mind. This way it becomes more clear that |
I copied how |
Great! This looks good to me. I'll merge it unless someone chimes in with more comments in a couple of days. |
Note: this builds on top of commits from #732. I will rebase after that PR lands.
Now that we have exposed the concept of
AsyncResource
toNan::Callback
andNan::AsyncWorker
, let's go ahead and deprecate the legacyCallback::Call
functions that do not accept a context.Related: deprecation of legacy
MakeCallback
in Node: nodejs/node#18632